-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Quadtree region class - QuadtreeGrid2D #115
Conversation
we need to replace that function with something that isn't specifically designed for the cartesian region. ultimately we want to provide a also, it looks like the build is failing because we haven't specified can you also explain a bit how this package called |
Thank you for your comments. I have a question. I understand that I need to write two different functions that say, e.g. "get_spatial_counts" and "get_spatio_magnitude_counts". However, I am not entirely sure where to put these functions? Should it be a part of Catalog class or QuadtreeGrid2D class? I guess in current implementation a wrapper function is called from catalog class. What is your opinion? |
good question. now that im thinking about it more, i think the best would be to modify the function in the catalog class to work with the region object directly. right now it calls this the general procedure of the function would be to use the |
I have identifed an issue with bin1d_vec() function. I am not sure whether this is actually an issue or not. it takes in mbins and magnitude and provides the index of the bin, where magnitude lies. from csep.utils.calc import bin1d_vec so it returns the magnitude bin of mm. idx = bin1d_vec(6, mbins, tol=0.00001, right_continuous=True) #This would give 0: Which is fine. But if, I give some magnitude that is below the mbins, e.g. mm=4 or 5. It would start negative counting beyond 5.95. idx = bin1d_vec(5, mbins, tol=0.00001, right_continuous=True) idx = bin1d_vec(4, mbins, tol=0.00001, right_continuous=True) Ideally, it should be returning zero. Is it something you desired? or is it a bug? |
this expected behavior is that if the value falls outside the range it returns -1. the downside is that this is a valid index in python, and some issues arised from returning None there. the fact its returning -10 is not expected, but practically should not cause any issues it should return -1. this only happens with the right_continuous flag. this issue only affects scalarr values, if you were to test with an array the |
This may cause problem for the following line. Because -1 or -10 are valid indices. So we may need additional check, whether catalog is filtered for the minimum magnitude or not. numpy.add.at(out, idx, 1) |
yes, you should check whether the bins values are correct. you will notice both of the spatial counts functions have some sort of check inside them. these checks also account for potentially masked cells. see the |
we should probably add a check to the magitude_counts function in the catalog class though. i think that is maybe where you found that line of code. |
I added the check to spatial_count and spatial_magnitude_count functions. So now it should work alright. I wanted to know, how do you treat the right corner event? I mean, the earthquakes occurring at lon of 180 or lat of 90? |
Now, I have completed integration of quadtree grid into pyCSEP. It is a basic working version for global testing region. Next steps are: |
great, lets go over it on the call tomorrow! can you also make sure to add mercantile (and any other extra requirements) to the requirements.txt and requirements.yaml files? before ill be able to merge, the build will need to be successful. |
the point at 180 should map to the bin at -180, but we should probably make an additional check, maybe in the constructor of the catalog class. this would enforce a convention about how lat/lon are reported by a catalog instead of trying to assume how to handle these points during binning. i'm not sure how exact we want to handle a lat of 90, because technically that is a point. conceptually, i think if an earthquake were to occur directly at lat = 90, we would want to place it in the [89.9, 90.0) bin. what do you think? |
I personally would like to go with the first suggestion. The point on 180 should belong to -180, both are same in actual. I would prefer to make this a constructor in Catalog class. May be change 180 to -180. What would you say? |
I added requirements of mercantile and area packages. But its still not resolving the issue of building package. |
it seems to be breaking during the cartopy build. yoou can look at the error message from the failing build by going here and looking through the terminal output from the build. i took a look and it seems like a cartopy issue, but take a look and let me know what you think. also, it looks like the |
I have a question. Would that be ethical and a good idea to directly copy the implementation of area from package to our implementation? If that is alright, then I can simply do that. Or if its a problem, then I would think of something else. The implementation is available here: |
that's a good question. from a legal standpoint, it depends on the license of the software package. i think that bsd-2 allows us to do this, see the license here. id recommend we write our own area calculation and associated unit-tests. im not normally about re-inventing the wheel, but this is a standad and easy enough to implement, we should probably do it. looking at their code, it seems like something we could easily implement as well. i tagged you in a pull request from @hanbao-ucla. i think we should implement a what do you think? |
Yeah that would be neat, as sometimes we need both 'rate [eg., eq/day/cell or eq/year/Earth]' and 'rate_density [eg. eq/year/km^2]' |
Hi @wsavran from csep.core.regions import CartesianGrid2D, Polygon, create_space_magnitude_region from csep.core.regions import QuadtreeGrid2D |
I wrote quadtree forecast reader in csep.utils.reader. Thus I needed to call QuadtreeGrid2D from csep.core.regions in readers, which caused circular imports. So I tried to rectify this by calling QuadtreeGrid2D inside the reader function. For now, I did this, as we agreed, but lets discuss sometime to improve structure of forecast reading. As the forecast handling remains same in general. We just have to call a different region, The forecast handling remains same overall. I believe forecast reading can also be done in the previous same function that you wrote, by introducing an if/else statement. |
As for documentation, I wrote documentation in docs/concepts/forecasts.rst and docs/concepts/regions.rst. I can not build them on my PC. I am not sure whether I was doing it the correct way or not. But I am sure, if you accept the changes in the docs and run "make html", it should work. |
hi @khawajasim, i got my github 2fa issues sorted and im back online. ill test out the build tonight and we can chat about it on the call tomorrow. thanks for spending some time to write the documentation. |
update docstrings for new functions minor text changes to quadtree documentation to fix references and other typos pin numpy version to <= 1.25.0
after more than enough time, im happy to say that im merging this pull request into the v0.6.0 release candidate branch |
* Quadtree region class - QuadtreeGrid2D (#115) * Added function to read QuadtreeGrid forecast * added spatial_count and spatio_mag_count functions * poission_evaluations using from QuadtreeGrid2D * unit tests for quadtree-grid * California quadtreegrid at L=12 added * california quadtreegrid loading function * setup.py modified-added mercantile * forecast.plot() for Single-res Quadtree * clean up unnecessary comments from code * Update python-app.yml (#170) * added documentation for roc and i1 score (#169) * Fixed dh issue by modifying get_bbox() (#175) Co-authored-by: khawajasim <asimkhawaja786@gmail.com> Co-authored-by: William Savran <wsavran@usc.edu>
QuadtreeGrid2D region class added along with some other helping functions.
I still need to make "_bin_catalog_spatial_counts(....)" work. For that I need to get idx_map, xs, ys and mask. Any suggestions for this?